Skip to content

Conversation

@Taneb
Copy link
Member

@Taneb Taneb commented Oct 31, 2025

Builds off #2852, continuing towards #2729 in bitesize chunks.

@Taneb Taneb marked this pull request as draft October 31, 2025 06:42
Copy link
Contributor

@jamesmckinna jamesmckinna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks great. I've made suggestions, but nothing is a deal-breaker.

quotientGroup : Group c (c ⊔ ℓ ⊔ c′)
quotientGroup = record { isGroup = quotientIsGroup }

project : Group.Carrier G Group.Carrier quotientGroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference is for π (for projection), echoing ι (for inclusion), but I won't fight for it...
... maybe I should?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got no strong feeling here

Taneb added 3 commits October 31, 2025 10:02
I noted that every time I used normal it was under sym

This felt like a good reason to reverse it
Comment on lines +29 to +31
private
module N = NormalSubgroup N
open NormalSubgroup N using (ι; module ι; conjugate; normal)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can simplify this to:

Suggested change
private
module N = NormalSubgroup N
open NormalSubgroup N using (ι; module ι; conjugate; normal)
private
open module N = NormalSubgroup N using (ι; module ι; conjugate; normal)

Comment on lines +35 to +37
abelian⇒subgroup-normal : {c′ ℓ′} Commutative G._≈_ G._∙_ (subgroup : Subgroup c′ ℓ′) IsNormal subgroup
abelian⇒subgroup-normal ∙-comm subgroup = record { normal = λ n g ∙-comm (ι n) g }
where open Subgroup subgroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd maybe be tempted to lift this out to Algebra.Construct.Quotient.AbelianGroup to bundle up the quotient-out-of-any-subgroup construction, and simplify the downstream imports in #2855 ?

New modules
-----------

* `Algebra.Cosntruct.Quotient.Group` for the definition of quotient groups.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Suggested change
* `Algebra.Cosntruct.Quotient.Group` for the definition of quotient groups.
* `Algebra.Construct.Quotient.Group` for the definition of quotient groups.

Comment on lines +114 to +115
project-isHomomorphism : IsGroupHomomorphism rawGroup (Group.rawGroup quotientGroup) project
project-isHomomorphism = record
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming?

Suggested change
project-isHomomorphism : IsGroupHomomorphism rawGroup (Group.rawGroup quotientGroup) project
project-isHomomorphism = record
project-isGroupHomomorphism : IsGroupHomomorphism rawGroup (Group.rawGroup quotientGroup) project
project-isGroupHomomorphism = record

Twofold reasons:

  • Subsequently, for Ring R / Ideal I, you also define project-isHomomorphism : IsRingHomomorphism, and maybe that's enough (let the X path context of the algebra determine what kind of IsXHomomorphism is defined
  • BUT, for Ring, what you actually need is to separate out the isMonoidHomomorphism field, so why not also isGroupHomomorphism?

Comment on lines +116 to +124
{ isMonoidHomomorphism = record
{ isMagmaHomomorphism = record
{ isRelHomomorphism = record
{ cong = ≈⇒≋
}
; homo = λ _ _ ≋-refl
}
; ε-homo = ≋-refl
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be lifted out as a separate (prior) definition

project-isMagmaHomomorphism : IsMagmaHomomorphism rawMagma (Group.rawMagma quotientGroup) project
project-isMagmaHomomorphism = record
  { isRelHomomorphism = record
    { cong = ≈⇒≋
    }
  ; homo = λ _ _  ≋-refl
  }

project-isMonoidHomomorphism : IsMonoidHomomorphism rawMonoid (Group.rawMonoid quotientGroup) project
project-isMonoidHomomorphism = record
  { isMagmaHomomorphism = project-isMagmaHomomorphism
  ; ε-homo = ≋-refl
  }

project-isGroupHomomorphism : IsGroupHomomorphism rawGroup (Group.rawGroup quotientGroup) project
project-isGroupHomomorphism = record
  { isMonoidHomomorphism = project-isMonoidHomomorphism
  ; ⁻¹-homo = λ _  ≋-refl
  }


module Algebra.NormalSubgroup {c ℓ} (G : Group c ℓ) where

open import Algebra.Definitions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused!

Comment on lines +61 to +63
≈⇒≋ : _≈_ ⇒ _≋_
≈⇒≋ {x} {y} x≈y = N.ε by trans (∙-cong ι.ε-homo x≈y) (identityˡ y)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
≈⇒≋ : _≈_ ⇒ _≋_
≈⇒≋ {x} {y} x≈y = N.ε by trans (∙-cong ι.ε-homo x≈y) (identityˡ y)

In view of the above refactoring of reflexivity...

Comment on lines +38 to +39
≋-refl : Reflexive _≋_
≋-refl {x} = N.ε by trans (∙-congʳ ι.ε-homo) (identityˡ x)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
≋-refl : Reflexive _≋_
≋-refl {x} = N.ε by trans (∙-congʳ ι.ε-homo) (identityˡ x)
≈⇒≋ : _≈_ ⇒ _≋_
≈⇒≋ {x} {y} x≈y = N.ε by trans (∙-cong ι.ε-homo x≈y) (identityˡ y)
≋-refl : Reflexive _≋_
≋-refl = ≈⇒≋ refl

Unsurprisingly, these proofs are definitionally equal to the previous versions, but put ≈⇒≋ ('abstract' reflexivity...) higher up the food chain.

open import Algebra.Morphism.Structures using (IsGroupHomomorphism)
open import Algebra.Properties.Monoid monoid
open import Algebra.Properties.Group G using (⁻¹-anti-homo-∙)
open import Algebra.Structures using (IsGroup)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this import? Cf. #2391 and see below.

}

quotientGroup : Group c (c ⊔ ℓ ⊔ c′)
quotientGroup = record { isGroup = quotientIsGroup }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now inline the definition above?

Suggested change
quotientGroup = record { isGroup = quotientIsGroup }
quotientGroup = record
{ isGroup = ...
}

jamesmckinna added a commit to jamesmckinna/agda-stdlib that referenced this pull request Nov 5, 2025
@jamesmckinna
Copy link
Contributor

jamesmckinna commented Nov 6, 2025

In lieu of any further review comments, please see #2859 (purely!) as a comparison point, and feel free to grab anything from there which you might find useful... it just seemed the easiest way to encapsulate all the thoughts I had had about your groups-rings-modules development

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants